Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Website-viewer #14

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Website-viewer #14

wants to merge 24 commits into from

Conversation

Sebastian-ubs
Copy link

@Sebastian-ubs Sebastian-ubs commented Nov 22, 2024

new pr as follow up of #13

This is the ground work for a generic, config driven online resource viewer extensions.

This includes

  • Showing how to add commands that spawn web pages inside of Platform
  • example web pages
  • reloading pages on scripture reference and scroll group changes, so that web pages stay up to date with the current scripture reference
  • retaining the web page on layout changes and restart of platform

👉 Demo

Screenshot 2024-11-22 112355


This change is Reviewable

@Sebastian-ubs
Copy link
Author

Sebastian-ubs commented Nov 22, 2024

interestingly github shows commits in order of their creation date, but the order I applied them on this branch is actually

  1. creating the extension with npm run create-extension -- website-viewer
  2. merging the relevant previously done work from web-viewer branch into this
  3. small fixes on top

image

@Sebastian-ubs
Copy link
Author

as mentioned in #13

Interesting files to review are

  • main.ts
  • webViewerOptions.ts

Maybe

  • types/online-resource-viewer.d.ts
  • contributions/menus.json
  • contributions/localizedStrings.json

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Thanks also for the rework to use create-extension. That's really helpful, and I appreciate your time. Just wanted to publish what I have reviewed so far since I need to shift gears. Will come back to this at some point

Reviewed 35 of 41 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: 37 of 41 files reviewed, all discussions resolved

@Sebastian-ubs
Copy link
Author

How to add a new resource: example 58aa039

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!! Thanks for the great work :) I have a few blocking comments, a non-blocking comment or two (you can resolve if you don't want to take action), and many info comments (no action needed unless you want to do something). Looking forward to having this in!

Reviewed 2 of 41 files at r1, 2 of 4 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Sebastian-ubs)


src/website-viewer/src/types/website-viewer.d.ts line 6 at r2 (raw file):

  export interface CommandHandlers {
    /**
     * Opens a new Website Viewer web view with a code sandbox mockup and returns the web view id

These commands don't open a new web view if there is already an existing one at that url, right? Probably worth mentioning here


src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

     * @returns
     */
    'dummy.dummy': () => Promise<string | undefined>;

If you are able to get rid of the SATISFY_TS_KEY thing like I suggested, you should hopefully be able to remove this as well


src/website-viewer/src/main.ts line 136 at r2 (raw file):

  scrollGroupRef: ScrollGroupScrRef | undefined,
): Promise<ScriptureReference> {
  if (scrollGroupRef === undefined) return papi.scrollGroups.getScrRef();

FYI passing undefined is the same as passing nothing for a parameter, so you could combine this line and the next into one :)

(please note there are many comments that are just informational comments. Don't feel any pressure to take action on them unless you want to)


src/website-viewer/src/main.ts line 191 at r2 (raw file):

  if (typeof oldRef === 'number' && typeof newRef === 'number') return oldRef !== newRef;
  // both no scroll group, need to detect object difference
  if (isScrRef(oldRef) && isScrRef(newRef)) return !deepEqual(oldRef, newRef);

FYI we have a platform-scripture-utils function we usually use for comparing two scrRefs called compareScrRefs. If it returns other than 0, they're not equal :)


src/website-viewer/src/main.ts line 199 at r2 (raw file):

async function getUserLanguageCode(): Promise<string> {
  // TODO: implement
  return 'NOT_YET_IMPLEMENTED';

FYI if you're looking for the interface languages, you can run papi.settings.get('platform.interfaceLanguage'). Note it is a string array, not just a string


src/website-viewer/src/main.ts line 212 at r2 (raw file):

  // When the scripture reference changes, re-render the last webview of type "websiteViewerWebViewType"
  // This is not fired in case of "no scroll group", this is handled inside the scroll group change code
  papi.scrollGroups.onDidUpdateScrRef((scrollGroupUpdateInfo: ScrollGroupUpdateInfo) => {

This and all these following events return unsubscribers that need to be run to deactivate your extension. Please add them to context.registrations :)


src/website-viewer/src/main.ts line 240 at r2 (raw file):

    if (!commandByWebViewId.has(webViewId)) return;

    const command = commandByWebViewId.get(webViewId) || SATISFY_TS_KEY;

I think you shouldn't need to use something like SATISFY_TS_KEY. Here, you can instead just return if command is undefined after this line.


src/website-viewer/src/main.ts line 283 at r2 (raw file):

  context.registrations.add(await websiteViewerWebViewProviderPromise);
  Promise.all(commandPromises)
    .then((arr) => context.registrations.add(...arr))

Though it is nice and convenient to use the old promise chain syntax sometimes, the code style guide says to avoid using it where possible unless there is a compelling reason. Please change this to use await and try/catch or add a compelling reason in a code comment


src/website-viewer/src/websiteViewerOptions.ts line 14 at r2 (raw file):

// satisfy typescript, although we do not expect these to appear
export const SATISFY_TS_KEY: keyof CommandHandlers = 'dummy.dummy';
export const SATISFY_TS_OPTIONS: WebsiteViewerOptions = {

This name is a bit confusing. This object is generally used as a sort of "default" object for WebsiteViewerOptions when there isn't another one, right? Maybe consider renaming DEFAULT_WEBSITE_VIEWER_OPTIONS?


src/website-viewer/src/websiteViewerOptions.ts line 24 at r2 (raw file):

export enum RefChange {
  DO_NOT_WATCH,

We have traditionally made enumeration members PascalCase :) but I don't think we have explicit style rules about it, so do what you want with this information


src/website-viewer/src/websiteViewerOptions.ts line 30 at r2 (raw file):

}

function range(start: number, end: number) {

Code style guide says to use verbs at the start of method names to make it clearer to understand what the variable is (I'd suggest maybe something like createRange or createArrayOfIntegersBetween). Additionally, I find this function hard to understand without reading the code because its name is not particularly descriptive. When that is the case, we usually try to make the method name more descriptive or put TSDocs on the method like this:

/**
 * Some description
 *
 * @param start description
...
 */

Note VS Code will make this for you and include the parameters for you if you type /** and press enter above the method


src/website-viewer/src/websiteViewerOptions.ts line 36 at r2 (raw file):

}

function englishBookNameMarble(scrRef: ScriptureReference) {

This and the following function also need verbs at the start. Maybe just add get at the start?


src/website-viewer/src/websiteViewerOptions.ts line 138 at r2 (raw file):

  };

  return new Map<keyof CommandHandlers, WebsiteViewerOptions>([

If you make this Map<keyof CommandHandlers | undefined, WebsiteViewerOptions>, it might make it so you don't have to use SATISFY_TS_KEY for this map

Copy link
Author

@Sebastian-ubs Sebastian-ubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 36 of 42 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)


src/website-viewer/src/main.ts line 136 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI passing undefined is the same as passing nothing for a parameter, so you could combine this line and the next into one :)

(please note there are many comments that are just informational comments. Don't feel any pressure to take action on them unless you want to)

done, thanks


src/website-viewer/src/main.ts line 191 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI we have a platform-scripture-utils function we usually use for comparing two scrRefs called compareScrRefs. If it returns other than 0, they're not equal :)

done - although I am wondering about the real difference. comparing the two objects seems to work similarly good.


src/website-viewer/src/main.ts line 199 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

FYI if you're looking for the interface languages, you can run papi.settings.get('platform.interfaceLanguage'). Note it is a string array, not just a string

❤️


src/website-viewer/src/main.ts line 212 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This and all these following events return unsubscribers that need to be run to deactivate your extension. Please add them to context.registrations :)

done


src/website-viewer/src/main.ts line 240 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think you shouldn't need to use something like SATISFY_TS_KEY. Here, you can instead just return if command is undefined after this line.

done


src/website-viewer/src/main.ts line 283 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Though it is nice and convenient to use the old promise chain syntax sometimes, the code style guide says to avoid using it where possible unless there is a compelling reason. Please change this to use await and try/catch or add a compelling reason in a code comment

I've changed it, please have a look if this is better now


src/website-viewer/src/websiteViewerOptions.ts line 14 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This name is a bit confusing. This object is generally used as a sort of "default" object for WebsiteViewerOptions when there isn't another one, right? Maybe consider renaming DEFAULT_WEBSITE_VIEWER_OPTIONS?

done


src/website-viewer/src/websiteViewerOptions.ts line 24 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We have traditionally made enumeration members PascalCase :) but I don't think we have explicit style rules about it, so do what you want with this information

done


src/website-viewer/src/websiteViewerOptions.ts line 30 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Code style guide says to use verbs at the start of method names to make it clearer to understand what the variable is (I'd suggest maybe something like createRange or createArrayOfIntegersBetween). Additionally, I find this function hard to understand without reading the code because its name is not particularly descriptive. When that is the case, we usually try to make the method name more descriptive or put TSDocs on the method like this:

/**
 * Some description
 *
 * @param start description
...
 */

Note VS Code will make this for you and include the parameters for you if you type /** and press enter above the method

done


src/website-viewer/src/websiteViewerOptions.ts line 36 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This and the following function also need verbs at the start. Maybe just add get at the start?

done


src/website-viewer/src/websiteViewerOptions.ts line 138 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

If you make this Map<keyof CommandHandlers | undefined, WebsiteViewerOptions>, it might make it so you don't have to use SATISFY_TS_KEY for this map

decided to return in the get calls instead, otherwise I have to do this handling for registering the command, which seems odd


src/website-viewer/src/types/website-viewer.d.ts line 6 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

These commands don't open a new web view if there is already an existing one at that url, right? Probably worth mentioning here

done


src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

If you are able to get rid of the SATISFY_TS_KEY thing like I suggested, you should hopefully be able to remove this as well

done, I was able to remove it, thanks.

Copy link
Author

@Sebastian-ubs Sebastian-ubs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you TJ. I hope I applied all review comments.
The remaining issue I see is with imports, where I am not sure how to resolve them.

Unable to resolve path to module 'papi-shared-types'.

'shared/services/scroll-group.service-model' import is restricted from being used by a pattern. Importing from this path is not allowed. Try importing from @papi/core. Imports from paths like 'shared', 'renderer', 'node', 'client' and 'main' are not allowed to prevent unnecessary import break.

'shared/services/web-view.service-model' import is restricted from being used by a pattern. Importing from this path is not allowed. Try importing from @papi/core. Imports from paths like 'shared', 'renderer', 'node', 'client' and 'main' are not allowed to prevent unnecessary import break.

Reviewable status: 36 of 42 files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your time and effort on the adjustments! It's very close :) just one more thing.

Reviewed 2 of 6 files at r3, 2 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)


src/website-viewer/src/main.ts line 191 at r2 (raw file):

Previously, Sebastian-ubs wrote…

done - although I am wondering about the real difference. comparing the two objects seems to work similarly good.

Thanks for the update. Using an existing function makes it easier to change how that function works everywhere all at once in the future. :) For example, if we one day need to do some slightly different comparison with it or something, we can do that. Like if we one day take versification into account.


src/website-viewer/src/main.ts line 283 at r2 (raw file):

Previously, Sebastian-ubs wrote…

I've changed it, please have a look if this is better now

Almost got it! Sorry, promises in TS are rather tricky. Doing a forEach on commandPromises will make it so the extension will finish activating before those promises resolve and add the unsubscribers to the registration inside the forEach because forEach doesn't work asynchronously; it all runs synchronously, so all the async function calls you're making inside it are not getting awaited anywhere. To properly wait on all the commandPromises, you can use Promise.all similarly to how you were doing it before. I think you should be able to do something like this:

context.registrations.add(
  ...(await Promise.all(commandPromises)),
  await websiteViewerWebViewProviderPromise,
  ...
);

This awaits all the commandPromises, then destructures the returned array of unsubscribers to be added to context.registrations :)


src/website-viewer/src/types/website-viewer.d.ts line 77 at r2 (raw file):

Previously, Sebastian-ubs wrote…

done, I was able to remove it, thanks.

Thank you for your effort on it! :)

tjcouch-sil
tjcouch-sil previously approved these changes Dec 2, 2024
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks again :) oh and thanks. And thank you! Oh and I just want to make sure I remembered to say thank you! Thank you!

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants